Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UDPSession and Listener should not close conns they did not create #165

Closed
Eudi4H opened this issue Mar 16, 2020 · 2 comments · Fixed by #167
Closed

UDPSession and Listener should not close conns they did not create #165

Eudi4H opened this issue Mar 16, 2020 · 2 comments · Fixed by #167

Comments

@Eudi4H
Copy link
Contributor

Eudi4H commented Mar 16, 2020

DialWithOptions creates a net.PacketConn for you. NewConn, NewConn2, and NewConn3 allow you to provide your own net.PacketConn. In either case, Close closes both the UDPSession and the net.PacketConn.
https://github.com/xtaci/kcp-go/blob/v5.5.9/sess.go#L373
I think that Close should not close the net.PacketConn if it was provided by the caller in NewConn, NewConn2, or NewConn3.

Compare with quic-go, which has a createdPacketConn boolean flag. DialAddr (equivalent of DialWithOptions) sets it to true. Dial (equivalent of NewConn2) sets it to false. Close closes the net.PacketConn only if the createdPacketConn flag is true.
https://github.com/lucas-clemente/quic-go/blob/v0.14.4/client.go#L21-L23
https://github.com/lucas-clemente/quic-go/blob/v0.14.4/client.go#L278-L281
https://github.com/lucas-clemente/quic-go/blob/v0.14.4/packet_handler_map.go#L190

Here is a sample program.

go.mod
module example.com/kcp-close

require (
	github.com/klauspost/cpuid v1.2.3 // indirect
	github.com/pkg/errors v0.9.1 // indirect
	github.com/tjfoc/gmsm v1.3.0 // indirect
	github.com/xtaci/kcp-go/v5 v5.5.9
	golang.org/x/crypto v0.0.0-20200311171314-f7b00557c8c4 // indirect
	golang.org/x/net v0.0.0-20200301022130-244492dfa37a // indirect
)
package main

import (
	"fmt"
	"io"
	"net"

	"github.com/xtaci/kcp-go/v5"
)

func acceptSessions(ln *kcp.Listener) error {
	for {
		conn, err := ln.AcceptKCP()
		if err != nil {
			return err
		}
		go func() {
			defer conn.Close()
			io.Copy(conn, conn)
		}()
	}
}

func main() {
	serverConn, err := net.ListenPacket("udp", "127.0.0.1:0")
	if err != nil {
		panic(err)
	}
	defer serverConn.Close()
	ln, err := kcp.ServeConn(nil, 0, 0, serverConn)
	if err != nil {
		panic(err)
	}
	go acceptSessions(ln)

	thirdPartyAddr, err := net.ResolveUDPAddr("udp", "8.8.8.8:53")
	if err != nil {
		panic(err)
	}

	// Create a net.PacketConn to give to kcp.NewConn2.
	clientConn, err := net.ListenPacket("udp", ":0")
	if err != nil {
		panic(err)
	}

	// Use the net.PacketConn before giving it to KCP.
	n, err := clientConn.WriteTo([]byte("test\n"), thirdPartyAddr)
	fmt.Printf("UDP WriteTo -> %v, %v\n", n, err)

	fmt.Println()

	// Create a kcp.UDPSession, use it, and close it.
	client, err := kcp.NewConn2(serverConn.LocalAddr(), nil, 0, 0, clientConn)
	if err != nil {
		panic(err)
	}
	n, err = client.Write([]byte("hello\n"))
	fmt.Printf("KCP Write -> %v, %v\n", n, err)
	var buf [10]byte
	n, err = client.Read(buf[:])
	fmt.Printf("KCP Read -> %q, %v\n", buf[:n], err)
	err = client.Close()
	fmt.Printf("KCP Close -> %v\n", err)

	fmt.Println()

	// net.PacketConn should still work after closing KCP.
	n, err = clientConn.WriteTo([]byte("test\n"), thirdPartyAddr)
	fmt.Printf("UDP WriteTo -> %v, %v\n", n, err)
}

The output is

UDP WriteTo -> 5, <nil>

KCP Write -> 6, <nil>
KCP Read -> "hello\n", <nil>
KCP Close -> <nil>

UDP WriteTo -> 0, write udp [::]:53657->8.8.8.8:53: use of closed network connection

I expected the output

UDP WriteTo -> 5, <nil>

KCP Write -> 6, <nil>
KCP Read -> "hello\n", <nil>
KCP Close -> <nil>

UDP WriteTo -> 5, <nil>
@Eudi4H
Copy link
Contributor Author

Eudi4H commented Mar 16, 2020

By the way, an easy workaround:

type nopCloser struct {
	net.PacketConn
}

func (c *nopCloser) Close() error {
	return nil
}
	client, err := kcp.NewConn2(serverConn.LocalAddr(), nil, 0, 0, &nopCloser{clientConn})

Eudi4H pushed a commit to Eudi4H/kcp-go that referenced this issue Mar 18, 2020
Eudi4H pushed a commit to Eudi4H/kcp-go that referenced this issue Mar 18, 2020
@Eudi4H Eudi4H changed the title UDPSession should not close conn if it did not create it UDPSession and Listener should not close conns they did not create Mar 18, 2020
@Eudi4H
Copy link
Contributor Author

Eudi4H commented Mar 18, 2020

While working on pull request #167 I realized that the same consideration applies to Listener as well.

xtaci pushed a commit that referenced this issue Mar 18, 2020
* Tests for closing owned/non-owned PacketConns (#165)

* Don't close non-owned conns in UDPSession and Listener (#165)

Co-authored-by: David Fifield <david@bamsoftware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant